Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH v1] dt-bindings: mmc: snps,dwcmshc-sdhci: fix
 rockchip,txclk-tapnum
To:     Rob Herring <robh+dt@kernel.org>
Cc:     heiko@sntech.de, shawn.lin@rock-chips.com, ulf.hansson@linaro.org,
        Jisheng.Zhang@synaptics.com, linux-mmc@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20210430072138.6537-1-jbx6244@gmail.com>
 <CAL_JsqKnD1t7oRDY5sTi3TjJ0vbFK0tQcVj1qCw_jiOfOQh=mA@mail.gmail.com>
From:   Johan Jonker <jbx6244@gmail.com>
Message-ID: <4b67c50e-558a-ab76-b8df-531691129837@gmail.com>
Date:   Tue, 4 May 2021 18:19:16 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:78.0) Gecko/20100101
 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <CAL_JsqKnD1t7oRDY5sTi3TjJ0vbFK0tQcVj1qCw_jiOfOQh=mA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/4/21 12:40 AM, Rob Herring wrote:
> On Fri, Apr 30, 2021 at 2:21 AM Johan Jonker <jbx6244@gmail.com> wrote:
>>
>> A test with the command below gives this error:
>> mmc@fe310000: rockchip,txclk-tapnum: missing size tag in [[8]]
>>
>> With this added to a dts file:
>> rockchip,txclk-tapnum = <0x8>;
>>
>> A look at the driver shows that:
>> DLL_TXCLK_TAPNUM_DEFAULT        0x8
>>
>> Adding the default value to the dts files is not needed.
>> Every clock is divided into 32 taps equally and
>> the max value is 31.
>>
>> Fix rockchip,txclk-tapnum property in snps,dwcmshc-sdhci.yaml by
>> adding a minimum, maximum and default.
>>
>> In the driver the function of_property_read_u8() is used,
>> but with dtbs_check the notifications only disappear in YAML
>> by changing uint8 to uint32.
> 

> Did you check what value the driver sees when you put 8 in the dts. I
> expect it will be 0 because you are reading the 1st byte in big
> endian.
> 

Hi,

Thanks for info.
A test with a dummy module shows that of_property_read_u8() read a 0
instead of 8.
There are approx 23 of_property_read_u8() functions in use in the
mainline kernel. Do they need a recheck?

Johan

=== other Rockchip recheck examples

drivers/mmc/host/sdhci-of-dwcmshc.c

	if (of_property_read_u8(mmc_dev(host->mmc)->of_node,
"rockchip,txclk-tapnum",
				&priv->txclk_tapnum))

drivers/pci/controller/pcie-rockchip-ep.c

	err = of_property_read_u8(dev->of_node, "max-functions",
				  &ep->epc->max_functions);

=== dts test
	dummy {
		compatible = "dummy";
		rockchip,dummy0x8 =        <0x8>;
		rockchip,dummy0x11223344 = <0x11223344>;
	};

=== driver probe
	of_property_read_u8(np,  "rockchip,dummy0x8", &tmp8);
	printk("u8 : 0x%08x\n", tmp8);
	of_property_read_u32(np, "rockchip,dummy0x8", &tmp32);
	printk("u32: 0x%08x\n", tmp32);

	of_property_read_u8(np,  "rockchip,dummy0x11223344", &tmp8);
	printk("u8 : 0x%08x\n", tmp8);
	of_property_read_u32(np, "rockchip,dummy0x11223344", &tmp32);
	printk("u32: 0x%08x\n", tmp32);

=== dmesg
[   18.089694] u8 : 0x00000000
[   18.090123] u32: 0x00000008
[   18.090494] u8 : 0x00000011
[   18.090865] u32: 0x11223344


>> The driver has no limit check for rockchip,txclk-tapnum.
>>
>> make ARCH=arm64 dtbs_check
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>> index e6c9a2f77..f43d8d829 100644
>> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>> @@ -48,7 +48,10 @@ properties:
>>
>>    rockchip,txclk-tapnum:
>>      description: Specify the number of delay for tx sampling.
>> -    $ref: /schemas/types.yaml#/definitions/uint8
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 31
>> +    default: 8
>>
>>
>>  required:
>> --
>> 2.11.0
>>