Received-SPF: pass (google.com: domain of linux-kernel+bounces-107689-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5dedcd26-ed59-415f-b978-87a546a0816d@opensynergy.com>
Date: Tue, 19 Mar 2024 16:05:55 +0100
Subject: Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.
Content-Language: en-US
To: Haixu Cui <quic_haixcui@quicinc.com>, Mark Brown <broonie@kernel.org>,
 Viresh Kumar <viresh.kumar@linaro.org>, linux-spi@vger.kernel.org,
 linux-kernel@vger.kernel.org
Cc: quic_ztu@quicinc.com, Matti Moell <Matti.Moell@opensynergy.com>,
 Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
References: <20240304154342.44021-1-Harald.Mommer@opensynergy.com>
 <20240304154342.44021-4-Harald.Mommer@opensynergy.com>
 <99afc631-c02b-42da-a8d4-87c65087f390@quicinc.com>
From: Harald Mommer <harald.mommer@opensynergy.com>
In-Reply-To: <99afc631-c02b-42da-a8d4-87c65087f390@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?WjJPU2hTN29KTXg4OHJ4ZFpVUmNPSklIcDVMdHBYMlNVckt6V2paQkNPTDNs?=
 =?utf-8?B?cVoxQjlYcW5oU3hvQmRuV0o1aVV1VnNXZU5BTHZpcGUrRnpQMms0R3JuSmVj?=
 =?utf-8?B?dmUwSjZwelZJSnQvUFIyVllEandHMXlaWjNaSGF3WDUwRUJwVHg5M1ZXOEtT?=
 =?utf-8?B?ZzkvS0VtWlB6ajhMVENHQzc0ZC9YRERMb2RsZmhweXNwS1BtU093K2htSk82?=
 =?utf-8?B?Y05SUVl5ZGk1aUJDQWQyT3BNWFRrLzNDaWwvSTA0N3RyZTUvMStZeE9hMit1?=
 =?utf-8?B?eXJza1p2MjRUMElLcTlXNGZ0YjM0aUJaaG1KRHMydWZaT21scEU2RlRpMERK?=
 =?utf-8?B?R2lRQnMwNGlMQm1VWllTZDdzd3RLQktyZXROdHdNZFRTY2xmaWVETElOMGg3?=
 =?utf-8?B?MUhCNE80ZVZMOThSS0o3NUp5TEl4RVBwMUVyZ3V6ZDJDV1FrYVNOZE5XWFcz?=
 =?utf-8?B?UkdDZ0hhTUF6MTMyQTNRTFY1Wmo5WHJWc3k3V05XMzhoNGZNM3pOdWR2WDd2?=
 =?utf-8?B?SnpDZlNMNWJ4MytDTUFKZURwSGJEN1p3Tkw0WUYwaUo0ZDRFVUlmdUtZek90?=
 =?utf-8?B?R3VPMWVpQkx4Mk9QY3R6VXVqb2tmYWk4WkNrc3dyWDhWUEMzV29rNm5YTnhm?=
 =?utf-8?B?VlFnRHZQWjRuVmluVS9USzFUd1JZTW9uWlRNeklJa0NSTEZnQW1PVDZCc2tk?=
 =?utf-8?B?cHJrRlRINVZiQmFNbkJYMjhEMitRVkNwc2dzQ3p6cmI0bU9nTnkrNTNabGVr?=
 =?utf-8?B?bE5aVGtUZi93ZVRPcUpqd1BTWUNOV0lnclpKT3RxVmtuWVpydW9iUS94OGw3?=
 =?utf-8?B?d1A3aUtvekFIakdLWEptcEdFSGlFNjVXaVJ2K2xHaDIrUGJWMUVWSWpWbm5J?=
 =?utf-8?B?eFl5R3BsU2NRY2lXb2hML3JDbVBla1YwcjdBdVJMOGY4aE4wbVlJT1NLT3pU?=
 =?utf-8?B?NnhzSXY2ekczY2JBU3MwU3Y3enBDMDc4REwwaWR1ZXhsQUxtN1ROZVRMcDhx?=
 =?utf-8?B?VUFsOXJ3QzRvdHl1Qzh2Wk1OL29JYS8wUTlzNFNCOTJLK3NMRTI5Y2daN1Y3?=
 =?utf-8?B?ME14SlRKK2JYMzhnMHBsVStsZ294QlBsZlJBMjFpYXl5Z04ybEQrMlZVK1hR?=
 =?utf-8?B?WWo5dzFzMGFXa3VBeTh6b0lGL0thdGtaSjhWSjQyUEdsQXNyNEVGZ3l6M2NL?=
 =?utf-8?B?aDZ4dzM1QjJ6YWRDbm5weG0yQmVOVUhsaHdHT1NsWDJPTWt3dXNOZkhnYWlq?=
 =?utf-8?B?d3c2RWloOGx0YlZ2V09LRkVXeHRweWlkWWtRVG5yWEVCRTRCK2pSd1hleTk3?=
 =?utf-8?B?WTl6VXdlSXlOTWpqbjdQZWFMbk5wTDVYT0oxcWMxaGtxbnpvMDBpQ0RIZTRp?=
 =?utf-8?B?SGx2UkQ3MXQ5WVpDbStRMDBRS0pseVdJSVBYQXpzVGJIMVJrNGY3c1pNVmdQ?=
 =?utf-8?B?R1lBTXczRGFvOUVWcE9kaFA5S0tjeVNzRC8zaFBJZnF1eURHeEl5Q1BjT0F1?=
 =?utf-8?B?RmtuV093a0JRR1UwdTVYcWFqK1lBeWIxdEt6Um82QXYrTjRoLzhzTFZlUHh0?=
 =?utf-8?B?cWY4TWJFWHNyeFF6SVZJTk1lM1c1dVZvaFlML2k5U3luK3JwSFViTE5wVVlN?=
 =?utf-8?B?a016a1MvL2swanZrYUpXTUc3WUQ0WllJbUt4ZlBObXJwQVJ2WCt1U09UM2sv?=
 =?utf-8?B?TFJjRTl1TjQ4YVQ3UHp4Qkl4cDNvV1cyVlhLYXdJMmhieXNMZDJlUXFPNjVh?=
 =?utf-8?B?ek42S01KR3U5R1ptWUxwdTRydkppZ09mK05Bd01zR0pTT0V6UUhtcnpNQzY0?=
 =?utf-8?B?dEFZcnE1eEVUWTgzTXBoaEx1akljRHVIYWJKWE5oWEJ4bGt6UUwwVWt5UmVJ?=
 =?utf-8?B?bWVKN281TzUvNjAyRVFFVXcxU2MrTUhJWEJXamFnQVdYNzh6WHZWZVg5YzJL?=
 =?utf-8?B?azNWQm42OXp4UStFM3E3YmJOOGNsYUtveVNEeUg0SXZydXNuNWJXbDlmUHEr?=
 =?utf-8?B?NTBObWpBbmtXbnZmU1dqL0pHc004RjlKMExHUDNkRGd1VU1LWTZBRy8zdEdS?=
 =?utf-8?B?WE5vVThqT25jZ3BoRXF0T29obGZWZ0JCN1dMRlFGM0Z5dzFGZGpHa0pXT21a?=
 =?utf-8?Q?CL2BB5jIXRZkGPw+Q6YsOWJLK?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 57966868-2082-4441-1aba-08dc48261523
X-MS-Exchange-CrossTenant-AuthSource: BEZP281MB2581.DEUP281.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 15:05:58.1526
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 800fae25-9b1b-4edc-993d-c939c4e84a64
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: j1JhC1bBqRJFL1X15uyenxRsHlKrtDYRleK0LhYnwCseQiQe+DUPvD3MLe382gSTpOl1fql5im+VNw3/89bBmA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: FR2P281MB2670
X-TM-AS-ERS: 104.47.11.169-0.0.0.0
X-TMASE-Version: StarCloud-1.3-9.1.1015-28262.000
X-TMASE-Result: 10--15.526400-4.000000
X-TMASE-MatchedRID: 0dFPYP4mu5T5ETspAEX/nkkXVQaP16N5gdXztqOEBnB4qfOd233bZa1E
	3eK47PQvHe/Upp/4ecTU2c00Whd2/4ms9BCunzDsDH4m2ujwCtuFEtM7u1PJt/1+yNkFY3qaP4o
	cZuwLw49Jx48TqNIdl7H+lELTBl4s/7zDDoXJoQ01wGjuQ0TIwysZXvcCxTRn5aLISmJsQyU8ng
	ic1rrgiDmZupOyubC2sg3rueGAZs7ZSZojEviuAjZ4NKr/TszH10/soz7yB7H0Ab8aOZu0ArRZU
	QZ32S1HbhkCAaAHjBZlvSMRVSGieWDT+Wb3xgCrwptnSAS/aGQDlcboR1QH12o6O2V7nV87CvBR
	h7+RgCUYXZOLopHn4G5Jc1hHAthj6/gZEQCNScu5kTI8BpRqcpbOkYpZpyLEp92zxqYVNnt+cbd
	mqOGVk8Ow65JkZhPUldYeoo7G9XitAMlP4gRdFiT5U3lJ4T8K89lXv/QVq2UiVDZDa7ea/d/upP
	exrMg481iVzvxB4Fg7uJKuSvzgWWkxewMv2is+aUtZwjXtiLO4QSeL4UvEmw==
X-TMASE-XGENCLOUD: f2ddcc2c-99d3-40ad-9b3c-0ec4eced5063-0-0-200-0
X-TM-Deliver-Signature: 8A1917135A07332EEA3056C31EDCFFF1
X-TM-Addin-Auth: S43XoeiJPsghTJDb0e+y9fvdLWyfPh4xtvqZq5KIwQ2PJFunNKQ38Xl0rFG
	LYUSkV5GavFkwGqsaLcrtVfHlZbj3M/vyRrqvLUWXwg2qCuGuPyqaU4eFo8F1u6Qerpe1mqkkLx
	qgnbBz0X6jI+hz0rEiS+Q4Ek7SG81sXiLjhGst2wKi2CS9e02WAeKrgR5KnOWDvtm1PJ3I5+S+W
	cFdxnI0J6504s21HAlDhKhjV2VgvuY1NMSUmyxBLq16hodf9gbxV2mMmYS24qYkRfGlT0N2XXCD
	tfFKHDyELAG0QOU=.NtuouYeR25rl1RhwqXgb7GAcjg8s1VhhiI13tYVh9WXXMvK3htd8gLUH0f
	pVPWULMAcYKscyHMFr95ij0GLCAUQfb9GwiYG9cT9WFL1piTfHGB9NcGV4nbdSQV/EeH5R11go7
	wWyEcSmKmscAs4Xo9zV1E4mOMYRSmvLIs7xWjmj7M8vxs/czMvDxDUNH/bUS0aYuUdek5mp51yI
	MGGcviltqyd+uFrCtm4CDH8/q0mxOCeh860zlZRL85bOJnF2WA/cFSSTitblKrexpOWbGLrDy19
	tQ0zsKKrO2eUx73zHtLCx2AJvJCOmNZSu7kl8QnN6CJp7P7aD3aI/bxV8Iw==
X-TM-Addin-ProductCode: EMS
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=opensynergy.com;
	s=TM-DKIM-20210503141657; t=1710860759;
	bh=KLyhDukYztrtgoTswqT1EaIckIl4RDO5+DpjXw5ERys=; l=4998;
	h=Date:To:From;
	b=bBIWj/BpUVIk9BqZq3rWnbi7z+ekyYwiq8tvb1N9tmEw9+QPay8+9P9b+yslB1sfh
	 raqRX9/L1TFemYbLuNQhdlJrpoOfQsh2TfXDh0jWwDpVDe7HjnrCNuSkcxKrB7Kryd
	 A0o8EMV21K1vdJQ5snTSIn2vdw+GfiaPq4SGYxDQmFxCekKlibUMWfHfJsLKrSptT9
	 s59lbTFKo9JY9odDt4KoBQ3I4zCaLc5oNzfJd8lOveyx9c8s9BF7DSyh0cSwqrHjrn
	 H1OmIAZ2WEph5njxiHEtTPpDQvyzmDGBj1bw+kGKOSL1surfU9jQXsCoDHyVdVueYM
	 FgKBlERHGDMmg==

Hello,

virtio-dev still down...

I have to admit when it comes to device trees it's most of the time a 
fight. You will see below, fighting.

On 18.03.24 10:39, Haixu Cui wrote:
> Hello,
>
>     As Mark recommended, I reserve the virtio_spi_probe function only 
> and list my comments.
>
> On 3/4/2024 11:43 PM, Harald Mommer wrote:
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +    struct device_node *np = vdev->dev.parent->of_node;
>> +    struct virtio_spi_priv *priv;
>> +    struct spi_controller *ctrl;
>> +    int err;
>> +    u32 bus_num;
>> +    u16 csi;
>> +
>> +    ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
>> +    if (!ctrl)
>> +        return -ENOMEM;
>> +
>> +    priv = spi_controller_get_devdata(ctrl);
>> +    priv->vdev = vdev;
>> +    vdev->priv = priv;
>> +    dev_set_drvdata(&vdev->dev, ctrl);
>
>     ctrl->dev.of_node is not set, so the child node cannot be parsed. 
> I would say, it's necessary to support the virtio spi device node in 
> the format:


What you most probably want to have here is

   ctrl->dev.of_node = np;

for reasons I don't understand completely yet but have some idea. I may 
play around to get it.


>
>     virtio-spi@4b013000 {
>         compatible = "virtio,mmio";
>         reg = <0x4b013000 0x1000>;
>         interrupts = <0 497 4>;
>
>         spi {
>             compatible = "virtio,device2d";
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             spidev {
>                 compatible = "xxx";
>                 reg = <0>;
>                 spi-max-frequency = <100000>;
>             };
>         };
>     };


Looks in my setup like this:

virtio_mmio@4b013000 {
   compatible = "virtio,mmio";
   reg = <0x0 0x4b013000 0x0 0x1000>;
   /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
   interrupts = <0 497 4>;
   spi,bus-num = <1234>; /* <<<=== This here has been added */
};

The "spi,bus-num" is missing in your setup so you use the default of 0.

For what is "reg = <0>;" good? Or is it a dummy and only has to be present?

I don't understand spi-max-frequency in the device tree entry as we get 
this value from the config space.


>> +
>> +    init_completion(&priv->spi_req.completion);
>> +
>> +    err = of_property_read_u32(np, "spi,bus-num", &bus_num);


np already set so this assignment should play no role here when reading 
the entry.


>> +    if (!err && bus_num <= S16_MAX)
>> +        ctrl->bus_num = (s16)bus_num;
>> +
>> +    virtio_spi_read_config(vdev);
>> +
>> +    ctrl->transfer_one = virtio_spi_transfer_one;
>> +
>> +    err = virtio_spi_find_vqs(priv);
>> +    if (err) {
>> +        dev_err(&vdev->dev, "Cannot setup virtqueues\n");
>> +        return err;
>> +    }
>> +
>> +    err = spi_register_controller(ctrl);
>> +    if (err) {
>> +        dev_err(&vdev->dev, "Cannot register controller\n");
>> +        goto err_return;
>> +    }
>> +
>> +    board_info.max_speed_hz = priv->max_freq_hz;
>> +    /* spi_new_device() currently does not use bus_num but better 
>> set it */
>> +    board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> +    /* Add chip selects to controller */
>
>         Creating the spi devices here statically, will introduce 
> conflicts if the same spi devices also in the device-tree.


Of course I need to create the SPI devices here statically. The 
"spi,bus-num = <1234>;" in the device tree has to be defined in a way 
that there are no conflicts. I do not understand this.


>
>> +    for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> +        dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> +        board_info.chip_select = csi;
>> +
>> +        if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
>> +            board_info.mode = SPI_MODE_0;
>> +        else
>> +            board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
>> +
>> +        if (!spi_new_device(ctrl, &board_info)) {
>> +            dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> +            spi_unregister_controller(ctrl);
>> +            err = -ENODEV;
>> +            goto err_return;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +err_return:
>> +    vdev->config->del_vqs(vdev);
>> +    return err;
>> +}
>> +
>

What I needed to support for our needs was a user space SPI interface. 
You probably have a different setup and additional more complex device 
tree configuration needs I do not understand (yet). I'm somewhat lost in 
the moment.