linux-kernel - Re: [PATCH v4 10/10] mtd: rawnand: stm32_fmc2: get resources from parent node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3377adc6-3e5e-b9b7-12be-c7aa44bfac82@st.com>
Date:   Mon, 11 May 2020 12:21:03 +0200
From:   Christophe Kerello <christophe.kerello@...com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
CC:     <richard@....at>, <vigneshr@...com>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <gregkh@...uxfoundation.org>,
        <boris.brezillon@...labora.com>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <devicetree@...r.kernel.org>, <marex@...x.de>
Subject: Re: [PATCH v4 10/10] mtd: rawnand: stm32_fmc2: get resources from
 parent node

Hi Miquel,

On 5/11/20 11:18 AM, Miquel Raynal wrote:
> Hi Christophe,
> 
> Christophe Kerello <christophe.kerello@...com> wrote on Wed, 6 May 2020
> 11:11:19 +0200:
> 
>> FMC2 EBI support has been added. Common resources (registers base
>> and clock) are now shared between the 2 drivers. It means that the
>> common resources should now be found in the parent device when EBI
>> node is available.
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@...com>
>> ---
> 
> [...]
> 
>> +
>> +static bool stm32_fmc2_nfc_check_for_parent(struct platform_device *pdev)
>> +{
>> +	u32 i;
>> +	int nb_resources = 0;
>> +
>> +	/* Count the number of resources in reg property */
>> +	for (i = 0; i < pdev->num_resources; i++) {
>> +		struct resource *res = &pdev->resource[i];
>> +
>> +		if (resource_type(res) == IORESOURCE_MEM)
>> +			nb_resources++;
>> +	}
>> +
>> +	/* Each CS needs 3 resources defined (data, cmd and addr) */
>> +	if (nb_resources % 3)
>> +		return false;
>> +
>> +	return true;
>> +}
> 
> This function looks fragile. Why not just checking the compatible
> string of the parent node?
> 

Yes, it is another way to check that we have an EBI parent node.

In this implementation, I was checking the number of reg tuples.
In case we have 6, it means that the register base address is defined in 
the parent node (EBI node).
In case we have 7, it means that the register base address is defined in 
the current node (NFC node).

>> +
>>   static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -1824,8 +1865,8 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
>>   	struct resource *res;
>>   	struct mtd_info *mtd;
>>   	struct nand_chip *chip;
>> -	void __iomem *mmio;
>>   	int chip_cs, mem_region, ret, irq;
>> +	int num_region = 1;
>>   
>>   	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>>   	if (!nfc)
>> @@ -1834,23 +1875,19 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
>>   	nfc->dev = dev;
>>   	nand_controller_init(&nfc->base);
>>   	nfc->base.ops = &stm32_fmc2_nfc_controller_ops;
>> +	nfc->has_parent = stm32_fmc2_nfc_check_for_parent(pdev);
>> +	if (nfc->has_parent)
>> +		num_region = 0;
>>   
>>   	ret = stm32_fmc2_nfc_parse_dt(nfc);
>>   	if (ret)
>>   		return ret;
>>   
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	mmio = devm_ioremap_resource(dev, res);
>> -	if (IS_ERR(mmio))
>> -		return PTR_ERR(mmio);
>> -
>> -	nfc->regmap = devm_regmap_init_mmio(dev, mmio, &stm32_fmc2_regmap_cfg);
>> -	if (IS_ERR(nfc->regmap))
>> -		return PTR_ERR(nfc->regmap);
>> -
>> -	nfc->io_phys_addr = res->start;
>> +	ret = stm32_fmc2_nfc_set_regmap_clk(pdev, nfc);
>> +	if (ret)
>> +		return ret;
> 
> Are you sure this driver sill works without the EBI block?
> 
> This change looks suspect.
> 

Yes, the driver works fine with current bindings and with EBI bindings.
In case we have an EBI parent node, it means that the register base 
address and the clock are defined in the parent node.
Without any EBI parent node, it means that the register base address and 
the clock are defined in the NFC node.

The new function stm32_fmc2_nfc_set_regmap_clk is looking for these 2 
resources in the NFC node or in the parent node.
static int stm32_fmc2_nfc_set_regmap_clk(struct platform_device *pdev,
					 struct stm32_fmc2_nfc *nfc)
{
	struct device *dev = &pdev->dev;
	struct resource res;
	int ret;

	if (nfc->has_parent)
		dev = dev->parent;

	ret = of_address_to_resource(dev->of_node, 0, &res);
	if (ret)
		return ret;

	nfc->io_phys_addr = res.start;

	nfc->regmap = device_node_to_regmap(dev->of_node);
	if (IS_ERR(nfc->regmap))
		return PTR_ERR(nfc->regmap);

	nfc->clk = devm_clk_get(dev, NULL);
	if (IS_ERR(nfc->clk))
		return PTR_ERR(nfc->clk);

	return 0;
}

Regards,
Christophe Kerello.

>>   
>> -	for (chip_cs = 0, mem_region = 1; chip_cs < FMC2_MAX_CE;
>> +	for (chip_cs = 0, mem_region = num_region; chip_cs < FMC2_MAX_CE;
>>   	     chip_cs++, mem_region += 3) {
>>   		if (!(nfc->cs_assigned & BIT(chip_cs)))
>>   			continue;
>> @@ -1888,10 +1925,6 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
>>   
>>   	init_completion(&nfc->complete);
>>   
>> -	nfc->clk = devm_clk_get(dev, NULL);
>> -	if (IS_ERR(nfc->clk))
>> -		return PTR_ERR(nfc->clk);
>> -
> 
> Same here
> 
>>   	ret = clk_prepare_enable(nfc->clk);
>>   	if (ret) {
>>   		dev_err(dev, "can not enable the clock\n");
>