Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dd6c7345-4635-dc26-dd25-153c87b4b336@ti.com>
Date:   Mon, 9 Jan 2023 10:34:18 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v2] mtd: spi-nor: macronix: add support for mx77l51250f
Content-Language: en-US
To:     Tomas Winkler <tomas.winkler@intel.com>,
        Tudor Ambarus <tudor.ambarus@microchip.com>,
        Pratyush Yadav <pratyush@kernel.org>,
        Michael Walle <michael@walle.cc>,
        <linux-mtd@lists.infradead.org>
CC:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        <linux-kernel@vger.kernel.org>,
        Alexander Usyskin <alexander.usyskin@intel.com>
References: <20230107214345.2524851-1-tomas.winkler@intel.com>
From:   Dhruva Gole <d-gole@ti.com>
In-Reply-To: <20230107214345.2524851-1-tomas.winkler@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey Tomas,

On 08/01/23 03:13, Tomas Winkler wrote:
> Add support for mx77l51250f spi-nor chips.
> 
> SPI NOR sysfs:
> 
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
> mx77l51250f
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
> c2751a
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
> macronix
> $ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 53464450060103ff00060110300000ffc2000104100100ff030001020001
> 00ff84000102c00000ffffffffffffffffffe520f3ffffffff1f44eb086b
> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ff8341bd0082a7
> 04db4403373830b030b0f7a9d55c009e29fff050f985ffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffff7f0ff0ff215cdcffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffff3c9b96f0c5a4c2ffffffffffffff
> ffff003600279df9c064fecfffffffffffff
> 
> $ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 36fb8e3e6f82c45bfabea45ec73c08a8
> /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 
> Note: The test_qspi.sh wasn not run as this device in intel setup is used
>        for BIOS and platform firmware storage.
Thanks for doing the testing but one small suggestion, move this below 
the "---" lines before sending the patch email.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
For the code and idea itself, pending the changes I have advised below.

Reviewed-by: Dhruva Gole <d-gole@ti.com>

> V2:
> 1. This chip supports SFDP, parse it instead of the manual configuration.

This sounds good! Do the other flashes in macronix_nor_parts support
this? Maybe they can be updated in some later patches as well if they do
support SFDP Parsing.

> 2. Add required output of sysfs attributes
Yes, but I am not sure in this is the way to do it in the commit body
itself.
When Tudor asked you for those test logs, I think he meant for you to
paste it in the cover letter, or below here not in the patch email. Not 
in the commit body.
Refer 
https://lore.kernel.org/all/cover.1661915569.git.Takahiro.Kuwano@infineon.com/
where Takahiro has provided the tested details for ID, SFDP, Test logs.
> 
>   drivers/mtd/spi-nor/macronix.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 6853ec9ae65d..6c3b4192a8ae 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -100,6 +100,8 @@ static const struct flash_info macronix_nor_parts[] = {
>   	{ "mx66u2g45g",	 INFO(0xc2253c, 0, 64 * 1024, 4096)
>   		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>   		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +	{ "mx77l51250f", INFO(0xc2751a, 0, 64 * 1024, 4096)
> +		PARSE_SFDP },
>   };
>   
>   static void macronix_nor_default_init(struct spi_nor *nor)

-- 
Thanks and Regards,
Dhruva Gole