Message-ID: <YMNQBpFX9DXcoGAc@vkoul-mobl>
Date:   Fri, 11 Jun 2021 17:29:02 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, Leon Romanovsky <leon@...nel.org>,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        hui.wang@...onical.com, Jason Gunthorpe <jgg@...dia.com>,
        Dave Ertman <david.m.ertman@...el.com>,
        sanyog.r.kale@...el.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH v4] soundwire: intel: move to auxiliary bus

On 09-06-21, 09:44, Pierre-Louis Bossart wrote:
> 
> 
> On 6/8/21 11:46 PM, Vinod Koul wrote:
> > Hi Pierre,
> > 
> > You might want to check your setting, this and some other mail (not all
> > though) sent by you seem to have landed up in my spam folder, dont know
> > why gmail is doing that...
> 
> I haven't changed any of my configurations, not sure what happens?
> 
> > On 01-06-21, 08:56, Pierre-Louis Bossart wrote:
> > > 
> > > > > b) Vinod commented:
> > > > > 
> > > > > "What I would like to see the end result is that sdw driver for Intel
> > > > > controller here is a simple auxdev device and no additional custom setup
> > > > > layer required... which implies that this handling should be moved into
> > > > > auxdev or Intel code setting up auxdev..."
> > > > > 
> > > > > I was unable to figure out what this comment hinted at: the auxbus is
> > > > > already handled in the intel_init.c and intel.c files and the auxbus is used
> > > > > to model a set of links/managers below the PCI device, not the controller
> > > > > itself. There is also no such thing as a simple auxdev device used in the
> > > > > kernel today, the base layer is meant to be extended with domain-specific
> > > > > structures. There is really no point in creating a simple auxbus device
> > > > > without extensions.
> > > > 
> > > > <back from vacations>
> > > 
> > > same here :-)
> > > 
> > > > I would like to see that the init_init.c removed completely, that is my
> > > > ask here
> > > > 
> > > > This layer was created by me to aid in creating the platform devices.
> > > > Also the mistake was not to use platform resources and instead pass a
> > > > custom structure for resources (device iomem address, irq etc)
> > > 
> > > We are 100% aligned on the ask to remove intel_init.c, this layer is
> > > unnecessary and adds more work for developers/maintainers. We will move all
> > > this in the SOF driver.
> > > 
> > > > I would like to see is the PCI/SOF parent driver create the sdw aux
> > > > device and that should be all needed to be done. The aux device would be
> > > > probed by sdw driver. No custom resource structs for resources please.
> > > I was following the previous paragraph but got stuck on the last sentence
> > > 'no custom structs for resources', see below.
> > > 
> > > > If that is not possible, I would like to understand technical details of
> > > > why that would be that case. If required necessary changes should be
> > > > made to aux bus to handle and not have sequencing issue which you had
> > > > trouble with platform approach.
> > > 
> > > I don't know what you are referring to with the 'sequencing issue which you
> > > had trouble with platform approach'. We never had any technical issues with
> > > platform devices, the solution works and has been productized. We are only
> > > doing this iso-functionality transition because GregKH asked us to do only
> > > use platform devices IF there is a real platform device (controlled by
> > > DT/ACPI).
> > > 
> > > I think we are also having language/specification issues here. I don't
> > > understand what you describe as a 'resource' - there is no interaction with
> > > firmware - nor how we can avoid being domain-specific for something that is
> > > Intel-specific.
> > > 
> > > Let's go back to the code to help the discussion: the auxiliary driver which
> > > manages a SoundWire link needs to be provided with a 'custom' structure that
> > > describes basic information provided by the PCI parent (link masks, quirks,
> > > IO register bases) and contains internal fields needed for the link
> > > management (mutex, ops, list, etc). This is the structure we use:
> > > 
> > > struct sdw_intel_link_res {
> > > 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
> > > 	void __iomem *registers;
> > > 	void __iomem *shim;
> > > 	void __iomem *alh;
> > 
> > These are resources and any auxiliary_device should add this. That way
> > while creating you can set up. Hint look at how platform_device sets up
> > resources
> 
> If you look at the *existing* code, we don't handle any "resources" with the
> platform devices, we use the platform_device_info.data to pass the link
> information. It's a void pointer. We do not touch the resource field in the
> platform_device_into at all.

Yes that is true I dont disagree on that part. My ask here is to make it
better, it can be followed up after this but I would at least like to
agree on the direction.

> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel_init.c#L168
>
> > > 	int irq;
> > 
> > irq is a generic field and should be again moved into auxiliary_device
> 
> It's information passed by the parent so that all links use the same irq. We
> added this maybe 1.5 years ago after spending months chasing race conditions
> that we could not root cause. there's nothing generic about this field.
> 
> > > 	const struct sdw_intel_ops *ops;
> > 
> > This is for callbacks right? Why cant the sdw aux driver call APIs
> > exported by SOF driver?
> 
> this is part of the context, this could be moved to a different structure.

ok

> > > 	struct device *dev;
> > 
> > Why do you need a dev pointer here? Is this parent or something else?
> 
> for convenience for runtime_pm, there are cases where the link can suspend
> but the parent has to remain active due to power rail dependencies, so we
> need to handle pm_runtime_get_noresume() and pm_runtime_put_noidle().
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
> 
> We already use this field *today*, this isn't new. I guess we could use
> dev->parent but that'd be a different patch.

Absolutely, that should be a different patch.

> 
> > > 	struct mutex *shim_lock; /* protect shared registers */
> > 
> > Okay so you serialize the access to shim across sdw and sof right?
> > export an api from sof driver and get rid of lock here
> 
> this is again something we do today. This is not a new field.
> 
> see the description here:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
> 
> This is not about serialization between SOF and SDW, only SDW drivers access
> the shim. It's about serialization between the different SDW driver
> instances accessing common hardware registers. Nothing new.

Yes this is existing and can be improved upon. I guess can be moved to
SOF driver?

> 
> > > 	u32 *shim_mask;
> > > 	u32 clock_stop_quirks;
> > > 	u32 link_mask;
> > > 	struct sdw_cdns *cdns;
> > > 	struct list_head list;
> > 
> > 
> > these sound as internal data to sdw instance, move into intel
> > driver instances
> 
> what intel driver?

Should have been clear, sdw intel driver

> 
> We have a PCI Intel driver for the parent (SOF) and a driver instance for
> each SoundWire link - probed when the parent creates the different SoundWire
> devices.
> 
> we need to have an Intel link driver which is different from the SOF driver
> used for the parent. This is information needed at the child level.
> 
> > > };
> > > 
> > > We could if it was desired for architectural clarity split this structure in
> > > what is provided by the parent and what is used inside of the auxiliary
> > > driver as an internal context that the parent doesn't touch, but these
> > > definitions are again Intel-specific.
> > 
> > So rather than think Intel specfic, I would suggest you think in generic
> > terms. You have a child auxiliary_device (think like PCI etc), add
> > the generic resources like iomem regions, irq etc and call into SOF
> > driver. That would make sdw driver look neat and help you get rid of
> > this bits
> 
> Not able to get what this means, sorry. the child device should not 'call
> into the SOF driver', mixing parent and child layers leads to disaster in
> general.
> 
> The model is exactly the same as what we have today with the platform
> devices. We did not add ANY new fields or information, what is passed in
> that structure is exactly the same as what we do upstream today with the
> platform devices.

Yes that is the correct thing to do from conversion point of view. But
as part of conversion, as a follow up patches I would like to see things
improved. My ask here again is to remove the init layer. I would have
liked the resources like irq and iomem ones moved into aux device, but
looks like consensus is that aux device will not support that!

> To make my point, here is the structure in intel.h as of v5.13-rc1
> 
> struct sdw_intel_link_res {
> 	struct platform_device *pdev;
> 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
> 	void __iomem *registers;
> 	void __iomem *shim;
> 	void __iomem *alh;
> 	int irq;
> 	const struct sdw_intel_ops *ops;
> 	struct device *dev;
> 	struct mutex *shim_lock; /* protect shared registers */
> 	u32 *shim_mask;
> 	u32 clock_stop_quirks;
> 	u32 link_mask;
> 	struct sdw_cdns *cdns;
> 	struct list_head list;
> };
> 
> and here's what we suggested in this patch:
> 
> struct sdw_intel_link_res {
> 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
> 	void __iomem *registers;
> 	void __iomem *shim;
> 	void __iomem *alh;
> 	int irq;
> 	const struct sdw_intel_ops *ops;
> 	struct device *dev;
> 	struct mutex *shim_lock; /* protect shared registers */
> 	u32 *shim_mask;
> 	u32 clock_stop_quirks;
> 	u32 link_mask;
> 	struct sdw_cdns *cdns;
> 	struct list_head list;
> };
> 
> You will notice that we removed the platform_device *pdev, but embedded this
> structure into a larger one to make use of container_of()
> 
> struct sdw_intel_link_dev {
> 	struct auxiliary_device auxdev;
> 	struct sdw_intel_link_res link_res;
> };
> 
> That's it. We did not change anything else, all the other fields are
> identical. We are only changing the TYPE of device and the interfaces for
> probe/remove but using the same information and the same device hierarchy.

The move in itself is okay but I dont think that should be the end goal.

-- 
~Vinod

Powered by blists - more mailing lists