| Message ID | 20211202113553.238011-8-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for MHI endpoint stack | expand |
On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote: > This commit adds support for creating and destroying MHI endpoint devices. > The MHI endpoint devices binds to the MHI endpoint channels and are used > to transfer data between MHI host and endpoint device. > > There is a single MHI EP device for each channel pair. The devices will be > created when the corresponding channels has been started by the host and > will be destroyed during MHI EP power down and reset. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/bus/mhi/ep/main.c | 85 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index ce0f99f22058..f0b5f49db95a 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -63,6 +63,91 @@ static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl) > return mhi_dev; > } > > +static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id) > +{ > + struct mhi_ep_device *mhi_dev; > + struct mhi_ep_chan *mhi_chan = &mhi_cntrl->mhi_chan[ch_id]; > + int ret; > + > + mhi_dev = mhi_ep_alloc_device(mhi_cntrl); > + if (IS_ERR(mhi_dev)) > + return PTR_ERR(mhi_dev); > + > + mhi_dev->dev_type = MHI_DEVICE_XFER; Elsewhere (at least in mhi_ep_process_tre_ring()) in your code you assume that the even-numbered channel is UL. I would say, either use that assumption throughout, or do not use that assumption at all. (I prefer the latter.) I don't really like how this assumes that the channels are defined in adjacent pairs. It assumes one is upload and the next one is download, but doesn't specify the order in which they're defined. If you're going to assume they are defined in pairs, you should be able to assume which one is defined first, and then simplify this code (and even verify that they are defined UL before DL, perhaps). > + /* Configure primary channel */ > + if (mhi_chan->dir == DMA_TO_DEVICE) { > + mhi_dev->ul_chan = mhi_chan; > + mhi_dev->ul_chan_id = mhi_chan->chan; > + } else { > + mhi_dev->dl_chan = mhi_chan; > + mhi_dev->dl_chan_id = mhi_chan->chan; > + } > + > + get_device(&mhi_dev->dev); > + mhi_chan->mhi_dev = mhi_dev; > + > + /* Configure secondary channel as well */ > + mhi_chan++; > + if (mhi_chan->dir == DMA_TO_DEVICE) { > + mhi_dev->ul_chan = mhi_chan; > + mhi_dev->ul_chan_id = mhi_chan->chan; > + } else { > + mhi_dev->dl_chan = mhi_chan; > + mhi_dev->dl_chan_id = mhi_chan->chan; > + } > + > + get_device(&mhi_dev->dev); > + mhi_chan->mhi_dev = mhi_dev; > + > + /* Channel name is same for both UL and DL */ You could verify the two channels indeed have the same name. -Alex > + mhi_dev->name = mhi_chan->name; > + dev_set_name(&mhi_dev->dev, "%s_%s", > + dev_name(&mhi_cntrl->mhi_dev->dev), > + mhi_dev->name); > + > + ret = device_add(&mhi_dev->dev); > + if (ret) > + put_device(&mhi_dev->dev); > + > + return ret; > +} > + > +static int mhi_ep_destroy_device(struct device *dev, void *data) > +{ > + struct mhi_ep_device *mhi_dev; > + struct mhi_ep_cntrl *mhi_cntrl; > + struct mhi_ep_chan *ul_chan, *dl_chan; > + > + if (dev->bus != &mhi_ep_bus_type) > + return 0; > + > + mhi_dev = to_mhi_ep_device(dev); > + mhi_cntrl = mhi_dev->mhi_cntrl; > + > + /* Only destroy devices created for channels */ > + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > + return 0; > + > + ul_chan = mhi_dev->ul_chan; > + dl_chan = mhi_dev->dl_chan; > + > + if (ul_chan) > + put_device(&ul_chan->mhi_dev->dev); > + > + if (dl_chan) > + put_device(&dl_chan->mhi_dev->dev); > + > + dev_dbg(&mhi_cntrl->mhi_dev->dev, "Destroying device for chan:%s\n", > + mhi_dev->name); > + > + /* Notify the client and remove the device from MHI bus */ > + device_del(dev); > + put_device(dev); > + > + return 0; > +} > + > static int parse_ch_cfg(struct mhi_ep_cntrl *mhi_cntrl, > const struct mhi_ep_cntrl_config *config) > { >
On Wed, Jan 05, 2022 at 06:28:08PM -0600, Alex Elder wrote: > On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote: > > This commit adds support for creating and destroying MHI endpoint devices. > > The MHI endpoint devices binds to the MHI endpoint channels and are used > > to transfer data between MHI host and endpoint device. > > > > There is a single MHI EP device for each channel pair. The devices will be > > created when the corresponding channels has been started by the host and > > will be destroyed during MHI EP power down and reset. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/bus/mhi/ep/main.c | 85 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 85 insertions(+) > > > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > > index ce0f99f22058..f0b5f49db95a 100644 > > --- a/drivers/bus/mhi/ep/main.c > > +++ b/drivers/bus/mhi/ep/main.c > > @@ -63,6 +63,91 @@ static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl) > > return mhi_dev; > > } > > +static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id) > > +{ > > + struct mhi_ep_device *mhi_dev; > > + struct mhi_ep_chan *mhi_chan = &mhi_cntrl->mhi_chan[ch_id]; > > + int ret; > > + > > + mhi_dev = mhi_ep_alloc_device(mhi_cntrl); > > + if (IS_ERR(mhi_dev)) > > + return PTR_ERR(mhi_dev); > > + > > + mhi_dev->dev_type = MHI_DEVICE_XFER; > > Elsewhere (at least in mhi_ep_process_tre_ring()) in your code > you assume that the even-numbered channel is UL. > > I would say, either use that assumption throughout, or do > not use that assumption at all. (I prefer the latter.) > > I don't really like how this assumes that the channels > are defined in adjacent pairs. It assumes one is > upload and the next one is download, but doesn't > specify the order in which they're defined. If > you're going to assume they are defined in pairs, you > should be able to assume which one is defined first, > and then simplify this code (and even verify that > they are defined UL before DL, perhaps). > Yes, the UL channel is always even numbered and DL is odd. I've removed the checks. > > + /* Configure primary channel */ > > + if (mhi_chan->dir == DMA_TO_DEVICE) { > > + mhi_dev->ul_chan = mhi_chan; > > + mhi_dev->ul_chan_id = mhi_chan->chan; > > + } else { > > + mhi_dev->dl_chan = mhi_chan; > > + mhi_dev->dl_chan_id = mhi_chan->chan; > > + } > > + > > + get_device(&mhi_dev->dev); > > + mhi_chan->mhi_dev = mhi_dev; > > + > > + /* Configure secondary channel as well */ > > + mhi_chan++; > > + if (mhi_chan->dir == DMA_TO_DEVICE) { > > + mhi_dev->ul_chan = mhi_chan; > > + mhi_dev->ul_chan_id = mhi_chan->chan; > > + } else { > > + mhi_dev->dl_chan = mhi_chan; > > + mhi_dev->dl_chan_id = mhi_chan->chan; > > + } > > + > > + get_device(&mhi_dev->dev); > > + mhi_chan->mhi_dev = mhi_dev; > > + > > + /* Channel name is same for both UL and DL */ > > You could verify the two channels indeed have the > same name. > done. Thanks, Mani
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index ce0f99f22058..f0b5f49db95a 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -63,6 +63,91 @@ static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl) return mhi_dev; } +static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id) +{ + struct mhi_ep_device *mhi_dev; + struct mhi_ep_chan *mhi_chan = &mhi_cntrl->mhi_chan[ch_id]; + int ret; + + mhi_dev = mhi_ep_alloc_device(mhi_cntrl); + if (IS_ERR(mhi_dev)) + return PTR_ERR(mhi_dev); + + mhi_dev->dev_type = MHI_DEVICE_XFER; + + /* Configure primary channel */ + if (mhi_chan->dir == DMA_TO_DEVICE) { + mhi_dev->ul_chan = mhi_chan; + mhi_dev->ul_chan_id = mhi_chan->chan; + } else { + mhi_dev->dl_chan = mhi_chan; + mhi_dev->dl_chan_id = mhi_chan->chan; + } + + get_device(&mhi_dev->dev); + mhi_chan->mhi_dev = mhi_dev; + + /* Configure secondary channel as well */ + mhi_chan++; + if (mhi_chan->dir == DMA_TO_DEVICE) { + mhi_dev->ul_chan = mhi_chan; + mhi_dev->ul_chan_id = mhi_chan->chan; + } else { + mhi_dev->dl_chan = mhi_chan; + mhi_dev->dl_chan_id = mhi_chan->chan; + } + + get_device(&mhi_dev->dev); + mhi_chan->mhi_dev = mhi_dev; + + /* Channel name is same for both UL and DL */ + mhi_dev->name = mhi_chan->name; + dev_set_name(&mhi_dev->dev, "%s_%s", + dev_name(&mhi_cntrl->mhi_dev->dev), + mhi_dev->name); + + ret = device_add(&mhi_dev->dev); + if (ret) + put_device(&mhi_dev->dev); + + return ret; +} + +static int mhi_ep_destroy_device(struct device *dev, void *data) +{ + struct mhi_ep_device *mhi_dev; + struct mhi_ep_cntrl *mhi_cntrl; + struct mhi_ep_chan *ul_chan, *dl_chan; + + if (dev->bus != &mhi_ep_bus_type) + return 0; + + mhi_dev = to_mhi_ep_device(dev); + mhi_cntrl = mhi_dev->mhi_cntrl; + + /* Only destroy devices created for channels */ + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) + return 0; + + ul_chan = mhi_dev->ul_chan; + dl_chan = mhi_dev->dl_chan; + + if (ul_chan) + put_device(&ul_chan->mhi_dev->dev); + + if (dl_chan) + put_device(&dl_chan->mhi_dev->dev); + + dev_dbg(&mhi_cntrl->mhi_dev->dev, "Destroying device for chan:%s\n", + mhi_dev->name); + + /* Notify the client and remove the device from MHI bus */ + device_del(dev); + put_device(dev); + + return 0; +} + static int parse_ch_cfg(struct mhi_ep_cntrl *mhi_cntrl, const struct mhi_ep_cntrl_config *config) {
This commit adds support for creating and destroying MHI endpoint devices. The MHI endpoint devices binds to the MHI endpoint channels and are used to transfer data between MHI host and endpoint device. There is a single MHI EP device for each channel pair. The devices will be created when the corresponding channels has been started by the host and will be destroyed during MHI EP power down and reset. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/bus/mhi/ep/main.c | 85 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)